-
Notifications
You must be signed in to change notification settings - Fork 208
PHPC-2117: Test on PHP 8.2 #1340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dc0756c
to
634b45b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments/questions but I don't think any changes are needed.
@@ -12,6 +12,9 @@ $m = create_test_manager(); | |||
|
|||
class MySubscriber implements MongoDB\Driver\Monitoring\CommandSubscriber | |||
{ | |||
private $startRequestId; | |||
private $startOperationId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -18,7 +18,6 @@ $types = array( | |||
foreach($types as $type) { | |||
// Use 16-byte data to satisfy UUID requirements | |||
$binary = new MongoDB\BSON\Binary('randomBinaryData', $type); | |||
$binary->foo = 'bar'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point - I'll duplicate the tests and have them create dynamic properties on PHP before 8.2. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated each test for PHP 8.2 and restored the originals 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this test is now restricted to PHP < 8.2, shouldn't we add back the dynamic property testing? This might have been an oversight.
Also, quoting version_compare
:
Note that pre-release versions, such as 5.3.0-dev, are considered lower than their final release counterparts (like 5.3.0).
So I think we do want to use <?php skip_if_php_version('>=', '8.1.99'); ?>
as discussed in #1340 (comment)
@@ -6,6 +6,8 @@ BSON encoding: Encoding objects into BSON representation | |||
require_once __DIR__ . '/../utils/basic.inc'; | |||
|
|||
class MyClass implements MongoDB\BSON\Persistable { | |||
private $props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1,5 +1,8 @@ | |||
--TEST-- | |||
PHPC-1598: BSON type get_gc should delegate to zend_std_get_properties | |||
--SKIPIF-- | |||
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?> | |||
<?php skip_if_php_version('>=', '8.1.99'); ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary since "< 8.2.0" would allow the test to run on pre-release versions, correct?
What's the actual reason that we can't run this on 8.2+? Is it due to dynamic creation of the a
property on each BSON object? I noted from https://wiki.php.net/rfc/deprecate_dynamic_properties that stdClass isn't restricted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary since "< 8.2.0" would allow the test to run on pre-release versions, correct?
I remember prior art for doing this, but I can also change it to < 8.2.0
.
What's the actual reason that we can't run this on 8.2+? Is it due to dynamic creation of the
a
property on each BSON object? I noted from https://wiki.php.net/rfc/deprecate_dynamic_properties that stdClass isn't restricted.
That is correct - starting with PHP 8.2 creating dynamic properties on classes emits a deprecation notice, with the functionality being removed in PHP 9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember prior art for doing this, but I can also change it to < 8.2.0.
Possible misunderstanding here, but wouldn't skip_if_php_version('>', '8.2.0');
cause test failures since the test would then be allowed to run on 8.2 pre-release versions? I think it's correct as-is, but was just asking for confirmation.
Note: the one test failure on server "latest" appears appears unrelated to PHPC-2075 and may warrant a new |
634b45b
to
abb62c4
Compare
abb62c4
to
c233d17
Compare
PHPC-2117